r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Oh getrandom updated the wasi dependency and that now includes a dependency on wit-bindgen-rt. That seems fine? I'll add it to the allowed deps but feel free to correct me!

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

☔ The latest upstream changes (presumably #136454) made this pull request unmergeable. Please resolve the merge conflicts.

Hm, so I don't have any hard objections to wit-bindgen-rt, but it's a pretty weird crate... it has no README on crates.io, and looking at the raw artifacts there seems to be C and some non-source files (wit-bindgen-rt-0.39.0/src/cabi_realloc.o, wit-bindgen-rt-0.39.0/src/libwit_bindgen_cabi_realloc.a) in it? That seems to get linked in on wasm32 targets, which feels somewhat surprising to me -- I wouldn't have expected binary artifacts in crates.io.

@alexcrichton could we perhaps add a README to the crate explaining why it has compiled artifacts? I suspect that's just going to reference the comment in https://github.com/bytecodealliance/wit-bindgen/blob/main/ci/rebuild-libcabi-realloc.sh, but since that file isn't shipped with the crate I had to do some hunting to find it :) It's my understanding that at least some upstream consumers of Rust don't want compiled artifacts in the "source" which this would be semi-introducing (it looks like miri already depends on this crate), I'm not sure how much of a problem that is in practice given what this is doing but it feels at least somewhat iffy.

@wesleywiser / @davidtwco as T-compiler leads probably worth taking a look. Should we have some policy around this sort of thing? Maybe there's some other way we can help achieve the goal of ~weak symbols this crate wants on stable?

$ tar tf wit-bindgen-rt-0.39.0.tar.gz
wit-bindgen-rt-0.39.0/.cargo_vcs_info.json
wit-bindgen-rt-0.39.0/Cargo.lock
wit-bindgen-rt-0.39.0/Cargo.toml
wit-bindgen-rt-0.39.0/Cargo.toml.orig
wit-bindgen-rt-0.39.0/build.rs
wit-bindgen-rt-0.39.0/src/async_support/future_support.rs
wit-bindgen-rt-0.39.0/src/async_support/stream_support.rs
wit-bindgen-rt-0.39.0/src/async_support.rs
wit-bindgen-rt-0.39.0/src/cabi_realloc.c
wit-bindgen-rt-0.39.0/src/cabi_realloc.o
wit-bindgen-rt-0.39.0/src/cabi_realloc.rs
wit-bindgen-rt-0.39.0/src/lib.rs
wit-bindgen-rt-0.39.0/src/libwit_bindgen_cabi_realloc.a

As one option I wonder if the pre-compiled sources could be replaced with a dependency on (for example) wasm-encoder and just producing them from the build script? That's likely to be slower to build but it would make confirming that the file is what it says it is to some extent easier.

Oh dear apologies if this is causing issues! As an internal implementation detail of wit-bindgen we didn't pay too much attention to public-facing documentation but I'm happy to add a README and/or crate docs pointing to the build script at the bare minimum.

It's my understanding that at least some upstream consumers of Rust don't want compiled artifacts in the "source"

If it helps at all we have documented/automated build instructions for this artifact in CI and it's always ensured that the checked-in copy is always up-to-date. This is intended to show that it's at least reproducible and matches the sources in tree. I realize though that such a setup may still not be sufficient for some consumers.

Maybe there's some other way we can help achieve the goal of ~weak symbols this crate wants on stable?

I can confirm that the feature we're lacking in Rust to avoid this artifact is indeed weak symbols. Specifically we want the ability to have a symbol, cabi_realloc, defined in multiple crates across a single link unit. The semantics of that function are defined outside of Rust so it doesn't matter who provides it, we just need to be sure that someone does. On the wasm32-wasip2 target the Rust standard library provides this symbol (as weak symbols can be used), so the purpose of wit-bindgen-rt is to provide the symbol for the wasm32-wasip1 target.

As one option I wonder if the pre-compiled sources could be replaced with a dependency on (for example) wasm-encoder and just producing them from the build script?

This is indeed possible! That's more-or-less what this build script does for an unrelated part of wasm. We manually assemble a libfoo.a with a specially-cratfted object file. The problem is that it's basically a write-once script that can't really be understood after-the-fact. If this is blocking an update for rand I can try to set aside time to do this, but I'd personally prefer to not go so far as it drastically reduces the maintainability of the crate. Basically I'd prefer to push on other avenues if possible, but if this is the only avenue then such is life sometimes!

Yeah, I agree that manually writing out the bytes is only sort of an improvement. It does feel plausible that in e.g. a reproducible builds or company import policy it would matter. One thought I did have is that you could plausibly produce the object "just in time" with a RUSTC_BOOTSTRAP rustc toolchain, but that doesn't seem like a great idea. Or maybe having some tool (a-la patchelf or a linker script or something) that edits the symbol a stable Rust compiler can produce.

I don't personally have a strong objection given the specific context here (and the lack of meaningful code in the blob) but it does seem like somewhat of a slippery slope.

In this particular context I suspect this dependency is never(?) actually used since rustc or miri probably never get built for wasm (at least not by us). So maybe we just say that makes this mostly a non-issue.

I'll nominate this for T-compiler to make a final call, and whatever we end up deciding we should probably make a policy note somewhere.

I do think a README would be good to add. I'll also note the published artifact doesn't have a LICENSE file (again, not sure it matters, and is probably not uncommon, but perhaps not strictly ideal).